Coverage decreased (-0.6%) to 90.415% when pulling d8c39a5 on Cito:convert_enums into 4827ce2 on graphql-python:master.

Coverage decreased (-0.6%) to 90.415% when pulling baa454d on Cito:convert_enums into 4827ce2 on graphql-python:master.

Coverage increased (+0.7%) to 91.64% when pulling 76b32ff on Cito:convert_enums into 4827ce2 on graphql-python:master.

Coverage increased (+0.3%) to 93.224% when pulling 361b44c on Cito:convert_enums into ef1fce2 on graphql-python:master.

Coverage increased (+0.7%) to 91.64% when pulling 76b32ff on Cito:convert_enums into 4827ce2 on graphql-python:master.

I think the enum should not be modified here (CamelCase type names and UPPERCASE options). Because it would be inconsistent with what graphene.Enum does.
Imagine that I have a python MyEnum, and use it in a column with sqlalchemy.Enum(MyEnum) and then I want a mutation that received this value.
I cannot do
value = graphene.Argument(graphene.Enum.from_enum(MyEnum)) because this is generating a different enum than the one based on the sqlalchemy column.

@Fedalto: Thanks for the feedback. We really need discussion, since this is complicated.

First, just to clarify: The CamelCase conversion is done to the enum type name, the UPPERCASE conversion happens to the enum key names. Graphene converts method names, but not type names, because Python has already the right name convention for these. However, databases usually have lower case type names for their enum types. Like in this example:

CREATE TYPE user_mood AS ENUM ('sad', 'ok', 'happy');

In SQLAlchemy you would write this type as

sqlalchemy.Enum('sad', 'ok', 'happy', name='user_mood')

It makes sense to convert this to a Graphene GraphQL Enum Type with he name UserMood and keys SAD, OK and HAPPY because that's what you expect in GraphQL, and on the other hand you don't want to have camelcase type and uppercase value names in the database. Graphene is not in the business of converting database types, so I don't think we can speak of an inconsistency here.

The complexity comes from the fact that we are dealing with 4 different kinds of enums: The actual database enum type (which might be a native type or just simulated with a check constraint), the SQLAlchemy Enum type that reflects it, the Python Enum that might (or might not) be underlying that SQLAlchemy Enum, and finally the graphene Enum GraphQL type.

Note that the SQLAlchemy Enum type must not necessarily be based off a Python Enum. You can define a SQLAlchemy Enum simply like in the example above, without using a Python Enum, or you can use a Python Enum like this:

PyUserMoodType = enum.Enum('UserMood', 'sad ok happy')
SaUserMoodType = sqlalchemy.Enum(PyUserMoodType)

In this case, the type name would be converted to usermood on the database.

My point is, when using graphene_sqlalchemy, we must use the SQLAlchemy Enum type as the source for converting to GraphQL, not the Python Enum type, because we might not even have one and because graphene_sqlalchemy operates one level higher in the abstraction.

The typical example is, like in your case, you have a property user_mood in an SQLAlchemy base class User like this:

UserMood = sqlalchemy.Enum('sad', 'ok', 'happy', name='user_mood')

class User(Base):
    __tablename__ = 'users'

    id = Column(Integer, primary_key=True)
    user_name = Column(String)
    user_mood = Column(UserMood)

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User

The question is how we can use the automatically created graphene enum type for the user mood in Arguments (like a filter in a query or in a mutation). We can't just redefine it with graphene.Enum because, as you noticed we then would have two different enum types. My solution was using the registry - that already exists in graphene_sqlalchemy - to not only store the types for sqlalchemy object types, but also for sqlalchemy enums. So we could do this:

from graphene_sqlalchemy.registry import registry

users = List(UserType, kind=Argument(registry.get_type_for_enum(UserMood)))

This would use the exact same GraphQL type for the argument as it is used on the column.

Alternatively, if you want to avoid using the registry, you can also do this:

users = List(UserType, kind=Argument(UserType._meta.fields['user_mood'].type))

I know this is a bit ugly (particularly because it is accessing a "private" attribute), but still readable. The expression simply gets the (GraphQL) type of the 'user_mood' field of the UserType GraphQL type.

Sorry for this lengthy explanation, but as I said this matter is complicated because of all the layers of abstraction and "impedence mismatch" on all the layers.

@Cito, thanks for the explanation.

Graphene converts method names, but not type names, because Python has already the right name convention for these.

I think I have a problem with this assumption. I'd not assume that a python enum will always have uppercase keys. So, I guess my problem is that this enum would result in lowercase keys:

graphene.Enum.from_enum(PyEnum('UserMood', 'sad ok happy')

but not when coming from a sqlalchemy enum.

What about the key and name changes be a responsibility of graphene.Enum?
That way, the GraphQL enum will always comply with the convention no matter the enum source of truth.

@Fedalto Thanks for the feedback.

It would be nice if we could shift the responsibility for name mangling to Graphene. But we can't do that since Graphene is a lower level interface and therefore does not and should not care. It takes the Enum type names and options exactly like you define them. This allows you to build anything with it. Note that while Graphene does in fact convert field names automatically, you can still override it by explicitly setting names on the fields.

However, Graphene-SQLAlchemy is a higher level interface that seeks to "translate" from SQL to GraphQL in a conventient and automatic fashion. Therefore, in my view, it should - at least by default - also translate the naming conventions, which are lowercase enum options and type names in SQL, but uppercase enum options and camel case type names in GraphQL. That way you get what you normally expect in each of the two worlds instead of forcing an alien naming convention to the other world.

Note that Graphene-SQLAlchemy also converts field names from snake_case to camelCase using the default conversion mechanism in Graphene, but contrary to when you use Graphene manually, you can't change that behavior - since the fields are created automatically, you have no chance to override the name parameter in that automatic process.

Of course, it would be nice if we could make the automatic conversion configurable on the level of the registry, class or field. On the class level, maybe we could use the Meta attribute, like this?

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User
        translate_field_names = True
        translate_enum_options = False
        translate_type_names = True

Any thoughts on if this is going to be merged or how should one handle enum's ?

@Cito @Fedalto @syrusakbary Any thoughts or recommendations on how one should proceed with implementing Enums as Graphene enums ?

I tried to use these auto-created enums with #87, and it fails. Is it supposed to be working though?

I am really looking forward to be able to use graphene-sqlalchemy fully, but with current 'Enums problems' and 'manually-generated mutations' it is a bummer :(

UPD By 'it fails' I mean the following error: ValueError: Expected XXX_SOME_FIELD_XXX to be Argument, but received Field. Try using Argument(XXX_MY_ENUM_NAME_XXX).

Hi, I'd love to know if this can be merged - @Cito, @Fedalto, @syrusakbary, any further thoughts from your perspective?

No further thoughts from my side. Would also love to see graphene-sqlalchemy moving forward.

Hello everyone,

I'm just catching up with this PR. Since everyone seems to agree that our current Enum situation is not ideal, it's time to make some progress on it.

From what I understand, the main contention point is around the automatic Enum values mangling. While I'm not a big fan of the automatic name mangling, I think it's fine in this case because it is consistent with the rest of graphene-sqlalchemy. Medium-term we should definitely figure out a way to opt-out of this. But I don't think we should block this PR for this because we can already manually override that behavior if we really need to by opting out of the automatic field generation:

class UserType(SQLAlchemyObjectType):
    class Meta:
        model = User
       exclude_fields = ('user_mood',)
    
   user_mood = graphene.Enum(UserMood)
   
  def resolve_user_mood(root, info, **args):
     ...

Let's plan to figure out how to cleanly opt-out of this Enum name mangling as part of a larger PR on how to selectively opt-out of all the name mangling done by graphene-sqlalchemy. There is already one PR addressing this: #178

@Cito Would you be interested in fixing the merge conflicts in the next few days? If not let me know and I'll take care of it.

Cheers,
J

@jnak - yes, this should move forward. I'look into this again in the next days..

@jnak - just pushed anew with some changes and based on the lastest master.

However, I noticed that meanwhile a function has been added to create special enums for sorting. That function caches the enums in a separate cache, not in the registry like my PR does.

I want to change that so that the sorting enums use the same caching mechanism in the registry. Also, I want to change the default function for creating the sort enum value names so that they also use uppercase. Will push another commit tomorrow with these changes.

@Cito Great. Let me know when it's ready for me to review

@jnak - I'm done with this now.

I have changed the API slightly by making the creation of sort enums a separate function. They are created automatically when requested, so you normally don't need manual creation. The old functions are still available with deprecation warnings.

@jnak Will be busy until next weekend. I suggest we create a different issue for refactoring the API and making it more consistent. We should decide whether this functionality should be implemented in the registry module or utility module, or maybe in a separate module, and whether/which registry functions we want to replicate in the utility module (a bit more convenient when using the global registry). Maybe we should create a special enum module with support functions for handling enums.

@jnak Let's move the discussion to #208, because this review really gets convoluted. I will then create a new PR based on the outcome of the discussion.

This has been superseded by #210 now after discussion in #208.